-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove DebugWithInfcx
machinery
#126075
Merged
Merged
Remove DebugWithInfcx
machinery
#126075
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
Jun 6, 2024
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #126108) made this pull request unmergeable. Please resolve the merge conflicts. |
r=me after rebase |
compiler-errors
force-pushed
the
remove-debugwithinfcx
branch
from
June 11, 2024 17:52
01082f6
to
bcf672a
Compare
@bors r=lcnr |
bors
added
S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed
S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Jun 11, 2024
This comment has been minimized.
This comment has been minimized.
whoops @bors r- |
bors
added
S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed
S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
Jun 11, 2024
compiler-errors
force-pushed
the
remove-debugwithinfcx
branch
from
June 12, 2024 01:30
bcf672a
to
ce09f5d
Compare
rustbot
added
the
WG-trait-system-refactor
The Rustc Trait System Refactor Initiative (-Znext-solver)
label
Jun 12, 2024
@bors r=lcnr |
bors
added
S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed
S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
Jun 12, 2024
This comment has been minimized.
This comment has been minimized.
compiler-errors
force-pushed
the
remove-debugwithinfcx
branch
from
June 12, 2024 02:13
ce09f5d
to
0fc18e3
Compare
@bors r=lcnr |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 12, 2024
…llaumeGomez Rollup of 9 pull requests Successful merges: - rust-lang#126039 (Promote `arm64ec-pc-windows-msvc` to tier 2) - rust-lang#126075 (Remove `DebugWithInfcx` machinery) - rust-lang#126228 (Provide correct parent for nested anon const) - rust-lang#126232 (interpret: dyn trait metadata check: equate traits in a proper way) - rust-lang#126242 (Simplify provider api to improve llvm ir) - rust-lang#126294 (coverage: Replace the old span refiner with a single function) - rust-lang#126295 (No uninitalized report in a pre-returned match arm) - rust-lang#126312 (Update `rustc-perf` submodule) - rust-lang#126322 (Follow up to splitting core's PanicInfo and std's PanicInfo) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 12, 2024
Rollup merge of rust-lang#126075 - compiler-errors:remove-debugwithinfcx, r=lcnr Remove `DebugWithInfcx` machinery This PR removes `DebugWithInfcx` after having a lot of second thoughts about it due to recent type system uplifting work. We could add it back later if we want, but I don't think the amount of boilerplate in the complier and the existence of (kindof) hacks like `NoInfcx` currently justify the existence of `DebugWithInfcx`, especially since it's not even being used anywhere in the compiler currently. The motivation for `DebugWithInfcx` is that we want to be able to print infcx-aware information, such as universe information[^1] (though if there are other usages that I'm overlooking, please let me know). I think there are probably more tailored solutions that can specifically be employed in places where this infcx-aware printing is necessary. For example, one way of achieving this is by implementing a custom `FmtPrinter` which overloads `ty_infer_name` (perhaps also extending it to have overrideable stubs for printing placeholders too) to print the `?u.i` name for an infer var. This will necessitate uplifting `Print` from `rustc_middle::ty::print`, but this seems a bit more extensible and reusable than `DebugWithInfcx`. One of the problems w/ `DebugWithInfcx` is its opt-in-ness. Even if a compiler dev adds a new `debug!(ty)` in a context where there is an `infcx` we can access, they have to *opt-in* to using `DebugWithInfcx` with something like `debug!(infcx.with(ty))`. This feels to me like it risks a lot of boilerplate, and very easy to just forget adding it at all, especially in cases like `#[instrument]`. A second problem is the `NoInfcx` type itself. It's necessary to have this dummy infcx implementation since we often want to print types outside of the scope of a valid `Infcx`. Right now, `NoInfcx` is only *partially* a valid implementation of `InferCtxtLike`, except for the methods that we specifically need for `DebugWithInfcx`. As I work on uplifting the trait solver, I actually want to add a lot more methods to `InferCtxtLike` and having to add `unreachable!("this should never be called")` stubs for uplifted methods like `next_ty_var` is quite annoying. In reality, I actually only *really* care about the second problem -- we could, perhaps, instead just try to get rid of `NoInfcx` and just just duplicate `Debug` and `DebugWithInfcx` for most types. If we're okay with duplicating all these implementations (though most of them would just be trivial `#[derive(Debug, DebugWithInfcx)]`), I'd be okay with that too 🤔 r? `@BoxyUwU` `@lcnr` would like to know your thoughts -- happy to discuss this further, mainly trying to bring this problem up [^1]: Which in my experience is only really necessary when we're debugging things like generalizer bugs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
WG-trait-system-refactor
The Rustc Trait System Refactor Initiative (-Znext-solver)
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes
DebugWithInfcx
after having a lot of second thoughts about it due to recent type system uplifting work. We could add it back later if we want, but I don't think the amount of boilerplate in the complier and the existence of (kindof) hacks likeNoInfcx
currently justify the existence ofDebugWithInfcx
, especially since it's not even being used anywhere in the compiler currently.The motivation for
DebugWithInfcx
is that we want to be able to print infcx-aware information, such as universe information1 (though if there are other usages that I'm overlooking, please let me know). I think there are probably more tailored solutions that can specifically be employed in places where this infcx-aware printing is necessary. For example, one way of achieving this is by implementing a customFmtPrinter
which overloadsty_infer_name
(perhaps also extending it to have overrideable stubs for printing placeholders too) to print the?u.i
name for an infer var. This will necessitate upliftingPrint
fromrustc_middle::ty::print
, but this seems a bit more extensible and reusable thanDebugWithInfcx
.One of the problems w/
DebugWithInfcx
is its opt-in-ness. Even if a compiler dev adds a newdebug!(ty)
in a context where there is aninfcx
we can access, they have to opt-in to usingDebugWithInfcx
with something likedebug!(infcx.with(ty))
. This feels to me like it risks a lot of boilerplate, and very easy to just forget adding it at all, especially in cases like#[instrument]
.A second problem is the
NoInfcx
type itself. It's necessary to have this dummy infcx implementation since we often want to print types outside of the scope of a validInfcx
. Right now,NoInfcx
is only partially a valid implementation ofInferCtxtLike
, except for the methods that we specifically need forDebugWithInfcx
. As I work on uplifting the trait solver, I actually want to add a lot more methods toInferCtxtLike
and having to addunreachable!("this should never be called")
stubs for uplifted methods likenext_ty_var
is quite annoying.In reality, I actually only really care about the second problem -- we could, perhaps, instead just try to get rid of
NoInfcx
and just just duplicateDebug
andDebugWithInfcx
for most types. If we're okay with duplicating all these implementations (though most of them would just be trivial#[derive(Debug, DebugWithInfcx)]
), I'd be okay with that too 🤔r? @BoxyUwU @lcnr would like to know your thoughts -- happy to discuss this further, mainly trying to bring this problem up
Footnotes
Which in my experience is only really necessary when we're debugging things like generalizer bugs. ↩